CORENET-6714: Enable Network Observability on Day 0#2752
CORENET-6714: Enable Network Observability on Day 0#2752OlivierCazade wants to merge 7 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@OlivierCazade: This pull request references CORENET-6714 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @OlivierCazade! Some important instructions when contributing to openshift/api: |
Review Summary by QodoAdd InstallNetworkObservability field for day-0 network observability
WalkthroughsDescription• Add InstallNetworkObservability optional boolean field to NetworkSpec • Field enables network observability by default when omitted or true • Update generated code and CRD manifests for new field • Support day-0 automatic Network Observability operator deployment Diagramflowchart LR
NetworkSpec["NetworkSpec CRD"] -- "adds field" --> InstallNetworkObservability["InstallNetworkObservability: *bool"]
InstallNetworkObservability -- "enables by default" --> NetworkObservability["Network Observability Operator"]
NetworkObservability -- "deploys at bootstrap" --> FlowCollector["FlowCollector"]
File Changes1. config/v1/types_network.go
|
Code Review by Qodo
1. InstallNetworkObservability not feature-gated
|
openshift-ci
bot
added
the
size/S
📝 WalkthroughWalkthroughThis pull request introduces a new feature gate called NetworkObservabilityInstall to the OpenShift platform. The changes add a new optional configuration field for network observability to the Network API specification, create a corresponding feature gate with tech preview availability, and establish CustomResourceDefinition manifests across multiple feature-set configurations (Default, DevPreviewNoUpgrade, TechPreviewNoUpgrade, OKD). The feature gate is enabled in TechPreviewNoUpgrade deployment modes and disabled by default in other configurations. Supporting test configurations and feature documentation are also included. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml (1)
115-119: Keep description in sync with Go type and apply same clarity improvement.The CRD schema correctly reflects the Go type. The description has the same ambiguity issue noted in the Go file—"it does nothing" should be clarified to "automatic installation is skipped."
Ensure this description stays synchronized with any documentation changes made to
config/v1/types_network.go.📝 Suggested description improvement
installNetworkObservability: description: |- - InstallNetworkObservability is an optional field that enables network observability - when omitted or set to true. If the field is set to false, it does nothing. + InstallNetworkObservability is an optional field that enables automatic installation + of network observability when omitted or set to true. If the field is set to false, + automatic installation is skipped (the operator may still be installed manually post-installation). + This field is immutable after installation. type: boolean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml` around lines 115 - 119, Update the CRD description for the installNetworkObservability field so it matches the Go type and the clearer wording used in config/v1/types_network.go: replace "it does nothing" with "automatic installation is skipped" and ensure the sentence indicates the field is optional and defaults to true when omitted; target the installNetworkObservability field description in the CRD and keep this text synchronized with the corresponding struct/comment in config/v1/types_network.go.config/v1/types_network.go (1)
90-94: Clarify documentation wording and consider adding immutability notice.Two suggestions for the field documentation:
The phrase "it does nothing" is ambiguous. Consider clarifying to: "If the field is set to false, automatic installation of network observability is skipped."
Based on the PR objective (Day 0 enablement), if this field is intended to be immutable after installation like other fields in
NetworkSpec(e.g.,ClusterNetwork,ServiceNetwork,NetworkType), the documentation should explicitly state this.📝 Suggested documentation improvement
- // InstallNetworkObservability is an optional field that enables network observability - // when omitted or set to true. If the field is set to false, it does nothing. + // InstallNetworkObservability is an optional field that enables automatic installation + // of network observability when omitted or set to true. If the field is set to false, + // automatic installation is skipped (the operator may still be installed manually post-installation). + // This field is immutable after installation. // // +optional InstallNetworkObservability *bool `json:"installNetworkObservability,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_network.go` around lines 90 - 94, Update the InstallNetworkObservability field comment to remove ambiguity and, if intended to be immutable after initial installation like other NetworkSpec fields, explicitly state that; specifically, change the sentence "If the field is set to false, it does nothing." to "If the field is set to false, automatic installation of network observability is skipped." and add a second sentence such as "This field is immutable after initial installation (Day 0) similarly to ClusterNetwork, ServiceNetwork, and NetworkType." Ensure the comment sits above the InstallNetworkObservability *bool field in NetworkSpec and references consistency with ClusterNetwork, ServiceNetwork, and NetworkType.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/v1/types_network.go`:
- Around line 90-94: Update the InstallNetworkObservability field comment to
remove ambiguity and, if intended to be immutable after initial installation
like other NetworkSpec fields, explicitly state that; specifically, change the
sentence "If the field is set to false, it does nothing." to "If the field is
set to false, automatic installation of network observability is skipped." and
add a second sentence such as "This field is immutable after initial
installation (Day 0) similarly to ClusterNetwork, ServiceNetwork, and
NetworkType." Ensure the comment sits above the InstallNetworkObservability
*bool field in NetworkSpec and references consistency with ClusterNetwork,
ServiceNetwork, and NetworkType.
In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml`:
- Around line 115-119: Update the CRD description for the
installNetworkObservability field so it matches the Go type and the clearer
wording used in config/v1/types_network.go: replace "it does nothing" with
"automatic installation is skipped" and ensure the sentence indicates the field
is optional and defaults to true when omitted; target the
installNetworkObservability field description in the CRD and keep this text
synchronized with the corresponding struct/comment in
config/v1/types_network.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f66427af-5dad-4322-a130-036759f2a2b3
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (2)
config/v1/types_network.gopayload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
config/v1/types_network.go
Outdated
| // InstallNetworkObservability is an optional field that enables network observability | ||
| // when omitted or set to true. If the field is set to false, it does nothing. | ||
| // | ||
| // +optional | ||
| InstallNetworkObservability *bool `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
1. installnetworkobservability not feature-gated 📘 Rule violation ✓ Correctness
A new field was added to the stable config.openshift.io/v1 NetworkSpec without +openshift:enable:FeatureGate=.... This makes the field unconditionally available on a stable v1 API, violating the requirement to gate new stable fields for controlled rollout.
Agent Prompt
## Issue description
A new field (`InstallNetworkObservability`) was added to the stable `config.openshift.io/v1` `NetworkSpec` without being gated by `+openshift:enable:FeatureGate=...`, which violates the requirement that new fields on stable v1 APIs must be behind a FeatureGate.
## Issue Context
This field currently appears in the ungated CRD manifest (`AAA_ungated.yaml`), meaning it is exposed unconditionally.
## Fix Focus Areas
- config/v1/types_network.go[90-94]
- config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yaml[116-120]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "installNetworkObservability": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "InstallNetworkObservability is an optional field that enables network observability when omitted or set to true. If the field is set to false, it does nothing.", | ||
| Type: []string{"boolean"}, | ||
| Format: "", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
2. Openapi json out of sync 🐞 Bug ✓ Correctness
The PR adds installNetworkObservability to the generated OpenAPI Go definitions, but the checked-in openapi/openapi.json still lacks the field under NetworkSpec, so OpenAPI consumers won’t see it and hack/verify-openapi.sh would diff-fail.
Agent Prompt
### Issue description
The PR updates `openapi/generated_openapi/zz_generated.openapi.go` to include the new `installNetworkObservability` field, but `openapi/openapi.json` is not updated and still lacks the field under `NetworkSpec`. This breaks consistency and will fail `hack/verify-openapi.sh`, and any tooling consuming `openapi/openapi.json` will miss the field.
### Issue Context
`hack/verify-openapi.sh` regenerates OpenAPI artifacts and diffs them against the committed versions.
### Fix Focus Areas
- openapi/openapi.json[8986-9034]
- hack/update-openapi.sh[9-13]
- hack/verify-openapi.sh[5-9]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
openshift-ci
bot
added
size/M
|
@OlivierCazade: This pull request references CORENET-6714 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@OlivierCazade: This pull request references CORENET-6714 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@OlivierCazade: This pull request references CORENET-6714 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml (1)
116-123: Description casing doesn't match enum values, and wording is ambiguous.Two issues with the description:
The description uses lowercase "enable" and "disable" but the enum values are CamelCase "Enable" and "Disable". This could confuse users when setting the field.
The phrase "it does nothing" is ambiguous. Based on the PR objectives, when set to "Disable", automatic installation is skipped. Consider clarifying to "network observability is not automatically installed" or similar.
Minor inconsistency: The existing
networkDiagnostics.modefield (line 144) uses"Disabled"(past tense), while this field uses"Disable"(present tense). Consider aligning with the existing pattern for consistency.📝 Suggested description improvement
description: |- - installNetworkObservability is an optional field that enables network observability - when omitted or set to enable. If the field is set to disable, it does nothing. - Valid values are "", "Enable", "Disable". + installNetworkObservability is an optional field that controls automatic installation + of the Network Observability operator during cluster creation. + When omitted or set to "Enable", network observability is automatically installed. + When set to "Disable", automatic installation is skipped (manual installation remains possible). + Valid values are "", "Enable", "Disable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml` around lines 116 - 123, The description for the installNetworkObservability field is inconsistent with its enum and ambiguous; update the description for installNetworkObservability to use the same casing as the enum values ("Enable", "Disable"), clearly state the behavior for each value (e.g., when set to "Enable" or omitted it enables automatic installation, when set to "Disable" network observability is not automatically installed), and consider aligning wording with the existing networkDiagnostics.mode choice (e.g., use "Disabled" vs "Disable") for consistency across the CRD.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml`:
- Around line 116-123: The description for the installNetworkObservability field
is inconsistent with its enum and ambiguous; update the description for
installNetworkObservability to use the same casing as the enum values ("Enable",
"Disable"), clearly state the behavior for each value (e.g., when set to
"Enable" or omitted it enables automatic installation, when set to "Disable"
network observability is not automatically installed), and consider aligning
wording with the existing networkDiagnostics.mode choice (e.g., use "Disabled"
vs "Disable") for consistency across the CRD.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f88d5b2c-e676-465b-b05d-b0d40c189d2f
⛔ Files ignored due to path filters (5)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (2)
config/v1/types_network.gopayload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/types_network.go
|
@OlivierCazade: This pull request references CORENET-6714 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| // Valid values are "", "Enable", "Disable". | ||
| // +kubebuilder:validation:Enum:="";Enable;Disable | ||
| // +optional | ||
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
"Disable" is misleading, because it is not disabling Network Observability; it does nothing. In the Enhancement Proposal, it uses the term "not enable" to mean it doesn't enable Network Observability.
What about using the values "true", "false" and ""? "false" would imply that it would not install Network Observability.
There was a problem hiding this comment.
We strongly discourage the use of booleans in OpenShift APIs because fields that start as booleans often evolve to need more than true || false as allowed values.
We prefer the use of enums that have meaningful values to end-users. I agree that installNetworkObservability: Enable | Disable is not very clear to an end user.
I did a quick read of the EP - do you envision there ever needing to be a way for customers to apply any custom configuration for the day 0 network observability enablement?
There was a problem hiding this comment.
Choosing common 'boolean' values like true/false, yes/no, and 0/1 is easier to remember than some arbitrary dual values like enable/disable. If I try to come up with another term besides "disable", they all sound bad like "null" or "no-op". I like yes/no as well, but it will have the same concerns you raised.
For day 0, the answer is no. Customers will likely need to make changes on day 2. Even if that were to change somewhere down the line, we would still want this knob to enable it separately. That's already the case for many features, which typically have an attribute named enable under the particular feature.
The bottom line is whether the attribute and the value are meaningful to the user, not the just the value itself. enable/disable by itself is no more clearer than true/false. It has to be associated with the attribute installNetworkObservability to determine whether it's clear or not. So ask yourself, "Is it clear what installNetworkObservability means when it is true or false?" My answer is yes, and that's because the attribute has a verb in there. If the attribute was NetworkObservability, then the values like enable is much better than true.
There was a problem hiding this comment.
For day 0, the answer is no. Customers will likely need to make changes on day 2. Even if that were to change somewhere down the line, we would still want this knob to enable it separately. That's already the case for many features, which typically have an attribute named enable under the particular feature
Good to know. The reason I asked is because if we were intending to enable some kind of custom configuration as part of day 0 installation, a simple enablement field may result in a slightly "clunkier" configuration experience in the future by having separate fields for stating installation intent and then custom configuration.
If you were intending to do day 0 custom configuration, I'd have suggested a structure more along the lines of:
networkObservability:
installationPolicy: Install | DoNotInstall | InstallWithCustomization
installWithCustomization:
# customizable fields here(note: naming is just for an example and not necessarily exactly what I would expect).
I also think that having a nested structure now is generally better for future flexibility rather than locking yourself into having multiple co-located *NetworkObservability fields in the future. In my experience, that starts to get confusing for end-users.
The bottom line is whether the attribute and the value are meaningful to the user, not the just the value itself. enable/disable by itself is no more clearer than true/false. It has to be associated with the attribute installNetworkObservability to determine whether it's clear or not. So ask yourself, "Is it clear what installNetworkObservability means when it is true or false?" My answer is yes, and that's because the attribute has a verb in there. If the attribute was NetworkObservability, then the values like enable is much better than true.
Sure. So what happens if in the future you'd like to add a different way to specify the installation of the network observability feature beyond pure yes/no semantics?
Something like:
networkObservabilityInstallationPolicy: Install | DoNotInstallgives you more flexibility in the future to add new values that are descriptive beyond True | False.
All this being said, if you folks feel strongly that true/false semantics provide the best experience for the end-user then 👍
There was a problem hiding this comment.
I appreciate the comments. There are no plans to extend this, because the thinking is that if you want do anything else, it should be done in the Network Observability Operator (NOO) on day 2.
To muddy the waters a bit more, I'm not that thrilled with the attribute name installNetworkObservability, because it doesn't just install NOO, it also "enables" it by configuring the custom resource (CR). Here are a few more suggestions:
installNetworkObservability: auto|manual # suggested by Olivier
networkObservabilityPolicy: enable|doNotEnable # camelCase, title case, or snake_case?
autoEnableNetworkObservability: true|false
autoEnableObservability: true|false # shortened since it's already under `networking`
|
@OlivierCazade All new fields must be feature gated and go through the standard feature promotion process. Please add a new feature gate, disabled by default, and gate this new field. More information on adding a new feature gate can be found here: https://github.com/openshift/api?tab=readme-ov-file#adding-new-featuregates More information on adding a new field behind a feature gate can be found here: https://github.com/openshift/api?tab=readme-ov-file#adding-new-fields |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_network.go`:
- Around line 89-95: This new InstallNetworkObservability field must be
introduced behind a feature gate: add a dedicated feature gate (e.g.,
"InstallNetworkObservability") and update the API gating so the field is
effectively disabled by default—annotate the InstallNetworkObservability field
in types_network.go to indicate it is feature-gated, add the feature name to the
repository's feature gate registry per FEATURE_GATES.md, enforce
admission/validation logic that rejects or ignores this field unless the gate is
enabled (update the relevant validation/strategy and any defaulting logic), and
add/update tests documenting the gate behavior so the field only takes effect
when the feature gate is explicitly enabled.
- Around line 90-95: The field InstallNetworkObservability has a mismatch
between its comment and its kubebuilder enum: the code comment lists
"Enable"/"Disable" but the annotation enforces "", "True", "False"; update the
comment to reflect the actual enum values (e.g., "Valid values are \"\",
\"True\", \"False\"") and clarify that an omitted value is treated as enabled if
desired; reference the InstallNetworkObservability field and the
kubebuilder:validation:Enum annotation to make the comment consistent with the
enforced values.
In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml`:
- Around line 115-124: The documentation for the installNetworkObservability CRD
field is out of sync with its enum: update the description in the
installNetworkObservability field to list the actual allowed enum values ("",
"True", "False") or alternatively change the enum to match the existing text;
locate the installNetworkObservability definition and make the description and
enum consistent (preferably change the description to "Valid values are \"\",
\"True\", \"False\"").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f2be5a44-6b34-41a6-a126-6eb3f5c4f667
⛔ Files ignored due to path filters (2)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (2)
config/v1/types_network.gopayload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml
|
|
||
| // installNetworkObservability is an optional field that enables network observability | ||
| // when omitted or set to enable. If the field is set to disable, it does nothing. | ||
| // Valid values are "", "Enable", "Disable". | ||
| // +kubebuilder:validation:Enum:="";True;False | ||
| // +optional | ||
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
New field should be gated by a feature gate.
Per reviewer everettraven's comment on this PR, new fields in OpenShift APIs should be introduced behind a feature gate (disabled by default) and go through the standard feature promotion process. This ensures the field can be safely introduced and tested before becoming generally available.
The repository has guidance on adding feature gates and gating fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_network.go` around lines 89 - 95, This new
InstallNetworkObservability field must be introduced behind a feature gate: add
a dedicated feature gate (e.g., "InstallNetworkObservability") and update the
API gating so the field is effectively disabled by default—annotate the
InstallNetworkObservability field in types_network.go to indicate it is
feature-gated, add the feature name to the repository's feature gate registry
per FEATURE_GATES.md, enforce admission/validation logic that rejects or ignores
this field unless the gate is enabled (update the relevant validation/strategy
and any defaulting logic), and add/update tests documenting the gate behavior so
the field only takes effect when the feature gate is explicitly enabled.
There was a problem hiding this comment.
This is correct and needs to be done. This cannot just go straight to GA.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Update the installNetworkObservability field to be explicitly marked as feature-gated, and correct the kubebuilder enum validation to match the documented/implemented values ("", Enable, Disable).
| // installNetworkObservability is an optional field that enables network observability | |
| // when omitted or set to enable. If the field is set to disable, it does nothing. | |
| // Valid values are "", "Enable", "Disable". | |
| // +kubebuilder:validation:Enum:="";True;False | |
| // +optional | |
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` | |
| // installNetworkObservability is an optional field that enables network observability | |
| // when omitted or set to Enable. If the field is set to Disable, it does nothing. | |
| // Valid values are "", "Enable", "Disable". | |
| // | |
| // This field is feature-gated and must not take effect unless the corresponding | |
| // feature gate is enabled. | |
| // +openshift:enable:FeatureGate=InstallNetworkObservability | |
| // +kubebuilder:validation:Enum:="";Enable;Disable | |
| // +optional | |
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` |
See review comment here
There was a problem hiding this comment.
@everettraven, thank you for confirming — agreed completely. Here's a concrete breakdown of what needs to be done to properly gate this field:
Required changes to introduce the feature gate
1. Register the new feature gate in features/features.go
Add an exported variable similar to how other gates are registered. A gate declaration in features/features.go should use newFeatureGate(...) chained with metadata such as reportProblemsToJiraComponent(...), contactPerson(...), productScope(...), enhancementPR(...), and enableIn(configv1.TechPreviewNoUpgrade), followed by mustRegister(). For example:
// FeatureGateInstallNetworkObservability enables automatic installation of
// the Network Observability operator during cluster bootstrap.
var FeatureGateInstallNetworkObservability = newFeatureGate("InstallNetworkObservability").
reportProblemsToJiraComponent("Network Observability").
contactPerson("OlivierCazade").
productScope(ocpSpecific).
enhancementPR("https://github.com/openshift/enhancements/pull/1908").
enableIn(configv1.TechPreviewNoUpgrade).
mustRegister()2. Annotate the field in config/v1/types_network.go
Add the +openshift:enable:FeatureGate marker to the field so the CRD generation tooling gates it properly:
// installNetworkObservability is an optional field that enables network observability
// when omitted or set to True. If the field is set to False, it does nothing.
// Valid values are "", "True", "False".
// +openshift:enable:FeatureGate=InstallNetworkObservability
// +kubebuilder:validation:Enum="";True;False
// +optional
InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"`3. Update features.md with a new table row documenting the gate.
4. Re-run code generation to update:
config/v1/zz_generated.featuregated-crd-manifests.yaml- The per-gate CRD variant files under
config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/ payload-manifests/crds/(the gated CRD payload variant)
Note: There is also a mismatch in the current diff — the
+kubebuilder:validation:Enummarker lists"";True;Falsewhile the comment above it documents"", "Enable", "Disable". These need to be reconciled before merging.
Good reference PRs for this pattern: #2587 (adding a feature gate for a new CRD) and #2590 (gating an existing field with +openshift:enable:FeatureGate=).
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| // installNetworkObservability is an optional field that enables network observability | ||
| // when omitted or set to enable. If the field is set to disable, it does nothing. | ||
| // Valid values are "", "Enable", "Disable". | ||
| // +kubebuilder:validation:Enum:="";True;False | ||
| // +optional | ||
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` |
There was a problem hiding this comment.
Description does not match enum values.
Same issue as the CRD: the comment on lines 90-92 documents "Enable", "Disable" but the kubebuilder annotation on line 93 enforces True, False. The description must be updated to match the actual enum values.
📝 Proposed fix to align comment with enum
- // installNetworkObservability is an optional field that enables network observability
- // when omitted or set to enable. If the field is set to disable, it does nothing.
- // Valid values are "", "Enable", "Disable".
+ // installNetworkObservability is an optional field that enables network observability
+ // when omitted or set to True. If the field is set to False, it does nothing.
+ // Valid values are "", "True", "False".
// +kubebuilder:validation:Enum:="";True;False
// +optional
InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_network.go` around lines 90 - 95, The field
InstallNetworkObservability has a mismatch between its comment and its
kubebuilder enum: the code comment lists "Enable"/"Disable" but the annotation
enforces "", "True", "False"; update the comment to reflect the actual enum
values (e.g., "Valid values are \"\", \"True\", \"False\"") and clarify that an
omitted value is treated as enabled if desired; reference the
InstallNetworkObservability field and the kubebuilder:validation:Enum annotation
to make the comment consistent with the enforced values.
| installNetworkObservability: | ||
| description: |- | ||
| installNetworkObservability is an optional field that enables network observability | ||
| when omitted or set to enable. If the field is set to disable, it does nothing. | ||
| Valid values are "", "Enable", "Disable". | ||
| enum: | ||
| - "" | ||
| - "True" | ||
| - "False" | ||
| type: string |
There was a problem hiding this comment.
Description does not match enum values.
The description on lines 117-119 states valid values are "", "Enable", "Disable", but the actual enum on lines 120-123 enforces "", "True", "False". This appears to be a leftover from before the commit that "Changed enum from Enable/Disable to True/False".
Users will receive validation errors if they follow the documented values.
📝 Proposed fix to align description with enum
installNetworkObservability:
description: |-
installNetworkObservability is an optional field that enables network observability
- when omitted or set to enable. If the field is set to disable, it does nothing.
- Valid values are "", "Enable", "Disable".
+ when omitted or set to True. If the field is set to False, it does nothing.
+ Valid values are "", "True", "False".
enum:
- ""
- "True"
- "False"
type: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@payload-manifests/crds/0000_10_config-operator_01_networks.crd.yaml` around
lines 115 - 124, The documentation for the installNetworkObservability CRD field
is out of sync with its enum: update the description in the
installNetworkObservability field to list the actual allowed enum values ("",
"True", "False") or alternatively change the enum to match the existing text;
locate the installNetworkObservability definition and make the description and
enum consistent (preferably change the description to "Valid values are \"\",
\"True\", \"False\"").
Add InstallNetworkObservability field to NetworkSpec to support automatic installation of the Network Observability operator during cluster creation. When omitted or set to true, the operator and FlowCollector are deployed at bootstrap time, enabling immediate network visibility for troubleshooting packet drops, latencies, and DNS tracking. When explicitly set to false, no automatic installation occurs (the operator can still be installed manually post-installation). This treats network observability as integral networking infrastructure, making it available by default rather than as a separate optional component.
openshift-ci
bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/tests/networks.config.openshift.io/NetworkObservabilityInstall.yaml (1)
7-17: Cover the opt-out branch too.
"Enable"is equivalent to the default/omitted behavior, so this only checks the non-distinguishing path. Please add a"Disable"round-trip as well, since that is the value that actually changes bootstrap behavior.➕ Suggested test case
tests: onCreate: - name: Should be able to set InstallNetworkObservability initial: | apiVersion: config.openshift.io/v1 kind: Network spec: installNetworkObservability: "Enable" expected: | apiVersion: config.openshift.io/v1 kind: Network spec: installNetworkObservability: "Enable" + - name: Should be able to disable InstallNetworkObservability + initial: | + apiVersion: config.openshift.io/v1 + kind: Network + spec: + installNetworkObservability: "Disable" + expected: | + apiVersion: config.openshift.io/v1 + kind: Network + spec: + installNetworkObservability: "Disable"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/networks.config.openshift.io/NetworkObservabilityInstall.yaml` around lines 7 - 17, Add a complementary test case that covers the opt-out branch by creating a second test (e.g., name it "Should be able to set InstallNetworkObservability to Disable") alongside the existing "Should be able to set InstallNetworkObservability" entry and provide both initial and expected YAML blobs with spec.installNetworkObservability: "Disable" so the Disable value round-trips and verifies the behavior that actually changes bootstrap behavior; ensure the new test mirrors the structure of the existing case (apiVersion/kind/spec) but uses "Disable" for both initial and expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_networks-TechPreviewNoUpgrade.crd.yaml`:
- Around line 116-125: Update the description for the CRD field
installNetworkObservability to replace the vague phrase "it does nothing" with
explicit bootstrap-only semantics: state that setting the field to "Disable"
skips automatic installation of network observability during cluster creation
(bootstrap), while the other values ("" or "Enable") allow automatic
installation, and that manual installation of network observability remains
possible later if "Disable" is used; keep the enum and type unchanged and ensure
the description clearly documents these behaviors for generated docs.
---
Nitpick comments:
In
`@config/v1/tests/networks.config.openshift.io/NetworkObservabilityInstall.yaml`:
- Around line 7-17: Add a complementary test case that covers the opt-out branch
by creating a second test (e.g., name it "Should be able to set
InstallNetworkObservability to Disable") alongside the existing "Should be able
to set InstallNetworkObservability" entry and provide both initial and expected
YAML blobs with spec.installNetworkObservability: "Disable" so the Disable value
round-trips and verifies the behavior that actually changes bootstrap behavior;
ensure the new test mirrors the structure of the existing case
(apiVersion/kind/spec) but uses "Disable" for both initial and expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: eda7df16-f86e-405a-bf1a-4445386b1ecf
⛔ Files ignored due to path filters (10)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/networks.config.openshift.io/NetworkObservabilityInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (17)
config/v1/tests/networks.config.openshift.io/NetworkObservabilityInstall.yamlconfig/v1/types_network.gofeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_networks-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_networks-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_networks-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_networks-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_networks-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/types_network.go
| installNetworkObservability: | ||
| description: |- | ||
| installNetworkObservability is an optional field that enables network observability | ||
| when omitted or set to enable. If the field is set to disable, it does nothing. | ||
| Valid values are "", "Enable", "Disable". | ||
| enum: | ||
| - "" | ||
| - Enable | ||
| - Disable | ||
| type: string |
There was a problem hiding this comment.
Clarify the bootstrap-only Disable semantics.
"it does nothing" is too vague for a public API field. Here, Disable is the only value that actually changes behavior, so the generated docs should say that it skips automatic installation during cluster creation and that manual installation is still possible later.
📝 Suggested wording
installNetworkObservability:
description: |-
installNetworkObservability is an optional field that enables network observability
- when omitted or set to enable. If the field is set to disable, it does nothing.
+ when omitted or set to Enable during cluster creation.
+ If the field is set to Disable, automatic installation is skipped; the operator can still
+ be installed manually later.
+ This field only affects bootstrap-time installation.
Valid values are "", "Enable", "Disable".📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| installNetworkObservability: | |
| description: |- | |
| installNetworkObservability is an optional field that enables network observability | |
| when omitted or set to enable. If the field is set to disable, it does nothing. | |
| Valid values are "", "Enable", "Disable". | |
| enum: | |
| - "" | |
| - Enable | |
| - Disable | |
| type: string | |
| installNetworkObservability: | |
| description: |- | |
| installNetworkObservability is an optional field that enables network observability | |
| when omitted or set to Enable during cluster creation. | |
| If the field is set to Disable, automatic installation is skipped; the operator can still | |
| be installed manually later. | |
| This field only affects bootstrap-time installation. | |
| Valid values are "", "Enable", "Disable". | |
| enum: | |
| - "" | |
| - Enable | |
| - Disable | |
| type: string |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@payload-manifests/crds/0000_10_config-operator_01_networks-TechPreviewNoUpgrade.crd.yaml`
around lines 116 - 125, Update the description for the CRD field
installNetworkObservability to replace the vague phrase "it does nothing" with
explicit bootstrap-only semantics: state that setting the field to "Disable"
skips automatic installation of network observability during cluster creation
(bootstrap), while the other values ("" or "Enable") allow automatic
installation, and that manual installation of network observability remains
possible later if "Disable" is used; keep the enum and type unchanged and ensure
the description clearly documents these behaviors for generated docs.
|
@OlivierCazade: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
config/v1: add installNetworkObservability field for day-0 installation
Summary
This PR adds the
installNetworkObservabilityfield to the Network CRD (config.openshift.io/v1) to support automatic installation of the Network Observability operator during cluster creation.Changes
InstallNetworkObservabilitystring field toNetworkSpecBehavior
When omitted or set to Enable, the Network Observability operator and FlowCollector are deployed at bootstrap time, providing immediate network visibility for troubleshooting packet drops, latencies, and DNS tracking.
When explicitly set to Disable, no automatic installation occurs (though the operator can still be installed manually post-installation).
Related
/cc @stleerh